Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[trace-debug] Added support for global locations #20821

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

awelc
Copy link
Contributor

@awelc awelc commented Jan 8, 2025

Description

As it appears, we actually need full support for global values. At least one (and immediate) reason for it is that we need to support reading a value from global location where it has been deposited as a result of a native function returning a value.

A particular example of this is related to dynamic fields where dynamic_fields::borrow_child_object returns a reference to a value that in some cases is NOT stored in a local variable before it is being picked up by a write to a different local.

Test plan

A new test has been added and all other existing tests must pass

@awelc awelc requested review from tzakian and dariorussi January 8, 2025 21:29
@awelc awelc self-assigned this Jan 8, 2025
Copy link

vercel bot commented Jan 8, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Jan 8, 2025 9:30pm
sui-kiosk ⬜️ Ignored (Inspect) Jan 8, 2025 9:30pm
sui-typescript-docs ⬜️ Ignored (Inspect) Jan 8, 2025 9:30pm

@awelc awelc temporarily deployed to sui-typescript-aws-kms-test-env January 8, 2025 21:30 — with GitHub Actions Inactive
Copy link
Contributor

@tzakian tzakian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Only question I have is how does scoping/lifetime ends work for globals? Will this be figured out by wherever that data is loaded into?

if (this.trace.events.length <= this.eventIndex + 1 ||
this.trace.events[this.eventIndex + 1].type !== TraceEventKind.CloseFrame) {
throw new Error('Expected an CloseFrame event after native OpenFrame event');
// process optional effects until reaching CloseFrame for the native function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@awelc
Copy link
Contributor Author

awelc commented Jan 9, 2025

Looks good to me.

Only question I have is how does scoping/lifetime ends work for globals? Will this be figured out by wherever that data is loaded into?

Indeed. Global locations are never shown in the UI and lifetimes are only needed for locals that do end up being shown there (so that we can stop showing them after they are no longer live)

@awelc awelc merged commit 560668e into main Jan 9, 2025
52 checks passed
@awelc awelc deleted the aw/trace-view-global-write-ref branch January 9, 2025 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants